Skip to content

Conversation

maheshrajus
Copy link
Contributor

…h ASF license

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

@ayushtkn ayushtkn requested a review from okumin June 16, 2025 20:02
@maheshrajus maheshrajus changed the title [WIP] HIVE-28984: Replace nashorn-core with graalvm which is compatible wit… HIVE-28984: Replace nashorn-core with graalvm which is compatible wit… Jun 17, 2025
@@ -44,6 +44,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@org.junit.Ignore("Replace nashorn-core with graalvm script engine")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no need to ignore the test takes care of ignoring if nashorn isn't in classpath

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case using "jdk.nashorn.internal.ir.debug.ObjectSizeCalculator" for calculating the object size.
For correcting this test case we have jira https://issues.apache.org/jira/browse/HIVE-28997 to address it. thx !

Copy link

<dependency>
<groupId>org.graalvm.js</groupId>
<artifactId>js</artifactId>
<version>${graalvm.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try (GraalJSScriptEngine se = GraalJSScriptEngine.create(null,
Context.newBuilder().allowExperimentalOptions(true)
.option("js.nashorn-compat", "true")
.allowAllAccess(true))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@okumin thanks for the review.
i checked and debug the above code. so here we are setting builder.allowAllAccess(true) in the method updateForNashornCompatibilityMode(). But this method call when contextConfig is null. But in our current case we are setting flags related to nashorn compatibility("js.nashorn-compat") in Context config and calling method GraalJSScriptEngine.create(). So we will not hit the null case(see code below) and the method(updateForNashornCompatibilityMode) will not call. So here setting "allowAllAccess(true)" in Context builder is ok in my opinion. Please let me know your opinion on same. thank you !

        if (contextConfig == null) {      --> in our current case this will not be NULL.
            contextConfigToUse = Context.newBuilder(new String[]{"js"}).allowExperimentalOptions(true);
            contextConfigToUse.option("js.syntax-extensions", "true");
            contextConfigToUse.option("js.load", "true");
            contextConfigToUse.option("js.print", "true");
            contextConfigToUse.option("js.global-arguments", "true");
            contextConfigToUse.option("js.charset", "UTF-8");
            if (NASHORN_COMPATIBILITY_MODE) {
                updateForNashornCompatibilityMode(contextConfigToUse);
            } else if (Boolean.getBoolean("graaljs.insecure-scriptengine-access")) {
                updateForScriptEngineAccessibility(contextConfigToUse);
            }
        }
    private static void updateForNashornCompatibilityMode(Context.Builder builder) {
        builder.allowAllAccess(true);
        builder.allowHostAccess(NASHORN_HOST_ACCESS);
        builder.useSystemExit(true);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll be right. What if .allowAllAccess(true) is removed? The document doesn't mention the option.

try (Context context = Context.newBuilder().allowExperimentalOptions(true).option("js.nashorn-compat", "true").build()) {
      context.eval("js", "print(__LINE__)");
  }

If GraalJS works with Hive without the option, it's better from the point of view of security.
https://www.graalvm.org/sdk/javadoc/org/graalvm/polyglot/Context.Builder.html#allowAllAccess(boolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i removed property .allowAllAccess(true) and checked it previously, we are getting error "failed due to: Unknown identifier: get". So in our case we need to pass this field. thanks !

@maheshrajus maheshrajus requested a review from okumin June 17, 2025 16:39
@maheshrajus
Copy link
Contributor Author

@okumin @ayushtkn Can you please review and approve the changes? thanks !

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ayushtkn ayushtkn merged commit fb87c7a into apache:master Jun 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants